It might be nice to rename these to the more explicit (if perhaps also longer) submit_command and cancel_command and remove the properties that point to them.

Okay, I'll try this, comming from Java background, it was more natural to me to override kind of methods instead of attributes :).

Most other dask related projects use the flake8 style checker, which would probably raise some warnings here for combining the if block into a single line. You might want to run the following from the root of this directory:

flake8 dask_jobqueue

But we should probably add this to CI so that we can automate this.

That's fine with me. Perhaps we could enable Travis CI to do some basic tests and style checking, without having all the docker testing env yet? Should we raise an issue separated from #2 ?

I've done this in #13. You can also run the same checker locally by running: flake8 -j auto dask_jobqueue

So far we've been using numpydoc style docstrings: http://dask.pydata.org/en/latest/develop.html#docstrings

I'm in favor of removing these properties and just going with class attributes if possible

Why remove threads_per_worker above? Also did you want to logically separate the dask options from the PBS options?

Ah, scrolling down I see that you've done just that. If so then local_directory should probably be an inherited parameter. I think it should be generic.

Yup I separated options, and for more consistancy with other keywords I used threads instead of thread_per_worker.
local_directory is generic, but I wanted to override it in PBSCluster, because standard PBS installation always provide a $TMPDIR variable which resolve to something like /tmp/{JOB_ID}/. I wanted to make this as the default local_directory for PBSCluster workers.

slightly safer to write this as:

self._command_template = os.path.join(dirname, 'dask-worker' % self.scheduler.address)

Let's just make this a class attribute that defaults to an empty string.

Okay, I tried to keep one of my "contract" of Abstract Class, but I guess it is still too Java oriented :).

pep8 (remove space after =)

I have a slight preference to set this to None by default. Either that or os.getenv('TMPDIR', None).

I believed that $TMPDIR was a PBS default, but apparently it should be PBS_TMPDIR. Can you check on the cluster you are using?
Maybe it is more generic to just remove this arg by default.

On our system (NCAR's cheyenne), TMPDIR is set to /var/tmp/{JOB_ID}. So given the divergence, I suggest just removing this as a default.

Then our systems are configured with the same variable (if not set to the same directory), it is difficult to generalise from two examples, but TMPDIR seems to be used for a scratch dir for you and me. Stilled removed it maybe I will add a comment for this.

I'm not totally sure what to do here. One option is to leave this as None and force (most) users to set this. Keeping it as is may be okay too but everyone will have to change it (except for you 😉). I have a slight preference for setting it to None.

Yep I have the same concern. I found your defaults too high, so I set mines, but it is probably wrong too.

On our cluster if we don't configure it, the default will be select=1:ncpus=1:mem=5GB. So worker jobs will probably be killed promptly if dask worker is set with more than 1 thread or process. And I believe it would be really bad for PBS to launch one job for each 1 process dask-worker.

My preference would be more close to what is mentioned in #12, if nothing is provided, resource_spec is computed using dask-worker options. Still not enthusiast about that? It's a good compromise, keep resource_spec keyword, but build it from dask-worker options if not provided. The only concern if for memory limit, there may be a need to extrapolate from String to integer...

So maybe it is simpler to set it to None :)...

We might inspect available memory and cpu cores using tools like psutil and multiprocessing

In [1]: import psutil
In [2]: psutil.virtual_memory().total
Out[2]: 16680607744
In [3]: psutil.cpu_count()
Out[3]: 4

That may be okay. The challenge is that you'll routinely have the situation where the machine that scheduler is running on is spec'd differently than the machines where the workers will be running on. I think this may be worth doing as a first guess.

I'm not inclined to base the resource_spec on the submitting node resource...
I would much prefer to guess it from the given threads and processes. Else I would default to None, maybe with a warning?

These will tend to be different on every machine - what about incorporating a user config file
that would allow local customization?

@jedwards4b Hmm, could you elaborate?
From my understanding of your suggestion, I would prefer to only have to change a keyword value than having a user config file, but it is just my feeling.

As for the comment from @mrocklin above, for me this could be put into the JobQueueCluster class, for choosing default processes and/or threads and memory. Maybe we ought to set those to None by default?
Commit incoming for demonstrating the automatic resource_spec assignment.

If we cannot reach a common view about this, I propose to continue discussing it in another issue, maybe #12 or a new one. Then for this pull request, I can put resource_spec to None by default, remove the solution I proposed with my last commit if you don't like it, and eventually merge all this?

These values are different from one machine to another, but typically don't need to change from one run to another on the same machine, sure a cli should be provided but a config file would allow the user not to have to remember and add the same cli again and again.

maybe say: Each option will be prepended with the #PBS prefix.

I think the initial implementation left those out but we should provide both. @jedwards4b - do you have a comment here?

see comment above. I think we'll want to include memory.

Can we add these options to the individual cluster doc strings?

Not sure I get your point, where do you want to put them? They are already in the JobQueueCluster doc strings. You wan to put it in every implementation?

My point was just that the JobQueueCluster doc string doesn't show up on the PBSCluster doc string so it requires some unnecessary searching to find all the possible arguments to the constructor.

There was already some duplicated doc, I updated it.
Does not feel really good about that though, don't like to duplicate docstrings... Is there a better way?

You could directly take the class docs and add to them. i.e. in each child class definition you do something like:

        doc = JobQueueCluster.__doc__
        self.__doc__ +=  "specific docs for Slurm cluster"

Perhaps there's a less hacky way though.

@rabernat - do I recall you introducing the xarray devs to a decorator approach to doc string templating? I can't seem to dig it up but perhaps someone knows what I'm talking about.

Comments like these can probably be removed

Removed.

And created a new issue #22

I'm curious if this name is perhaps a little ambiguous now. It might be worth it for the shorter name, I'm not sure.

If it would be me, I would name every corresponding attribute with suffix liker "per_worker" or prefix like "worker_". Like mentioned in #7 (comment).
As it stands, I don't see why threads should be treated differently than memory. Or maybe we should change memory to memory_per_worker, as for processes it actually translates in "number of workers"?